[fix](test) stabilize CreateResourceCommandTest#65767
Draft
shuke987 wants to merge 1 commit into
Draft
Conversation
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Collaborator
Author
|
run buildall |
Collaborator
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Automated review summary
Overall: No blocking issue found. This test-only change removes the process-wide JMockit mocks, keeps the real FE Env, and limits the test double to the single ADMIN decision exercised by testValidate; production resource behavior is unchanged.
Critical checkpoints:
- Correctness and test intent: the Mockito matchers bind to the exact
checkGlobalPriv(ConnectContext, ADMIN)call used byCreateResourceInfo.validate(), so the normal and invalid-property/type assertions still reach their intended validation paths. - Lifecycle and concurrency: the injected spy remains for the rest of this test class, but FE UT classes run in non-reused Surefire forks, the only later method uses the root context whose real manager also grants ADMIN, and the spy delegates all other calls. No observable same-class, cross-class, or daemon-race defect was substantiated.
- Compatibility, security, and performance: only test code changes; no production API, persistence, authorization, runtime, or resource-type behavior changes.
- Test coverage and validation: the full one-file diff and both test paths were reviewed statically. Per the PR description, the exact FE test class and full Maven reactor passed twice; this review runner did not rerun builds or tests.
- Documentation and release notes: no documentation or release note is needed for this test stabilization.
User focus: No additional user-provided focus was supplied; the full PR was reviewed.
Conclusion: comment-only approval opinion with no inline issues.
Contributor
FE UT Coverage ReportIncrement line coverage `` 🎉 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: DORIS-26901
Related PR: #65072, #62221
Problem Summary:
CreateResourceCommandTeststarts a real FE, buttestValidateglobally mocksEnvwith JMockit. FE background daemons concurrently callEnv.getServingEnv().isReady()while the test records expectations. JMockit's process-wide record state is then contaminated, causing intermittentMissingInvocation, incompatible return type, andConcurrentModificationExceptionfailures before the resource validation assertions run.The failure appeared in 11 of 80 branch-4.1 FE UT occurrences across 7 unrelated PRs. #65072 retained/widened the global JMockit setup on branch-4.1, while #62221 already uses the narrow Mockito pattern on master.
This PR keeps the real FE
Env, spies onlyAccessControllerManager, stubs the singleADMINprivilege decision needed by the test, and injects the spy into the real environment. It removes the global JMockit mocks forEnv,AccessControllerManager, andSystemInfoService.No production code or resource validation assertion is changed.
Validation:
./run-fe-ut.sh --run org.apache.doris.nereids.trees.plans.commands.CreateResourceCommandTestmvn test -Dcheckstyle.skip=true -DfailIfNoTests=false -Dtest=org.apache.doris.nereids.trees.plans.commands.CreateResourceCommandTestgit diff --checkpassed.Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)